-
Notifications
You must be signed in to change notification settings - Fork 1.6k
User/beenachauhan/test wsladiag #13975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/wsl-for-apps
Are you sure you want to change the base?
User/beenachauhan/test wsladiag #13975
Conversation
test/windows/WsladiagTests.cpp
Outdated
|
|
||
| const bool noSessions = (out.find(L"No WSLA sessions found.") != std::wstring::npos); | ||
|
|
||
| const bool hasTable = (out.find(L"WSLA session") != std::wstring::npos) && (out.find(L"ID") != std::wstring::npos) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend having a utility method to verify the complete output. Something like:
ValidateOutput("list", "No WSLA sessions found.\r\n");
For instance. That would make writing future tests easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same for below tests)
test/windows/WsladiagTests.cpp
Outdated
|
|
||
| static std::wstring BuildWsladiagCmd(const std::wstring& args) | ||
| { | ||
| const auto exePath = wsl::windows::common::wslutil::GetBasePath() / L"wsladiag.exe"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetBasePath() won't work here since it will look up the current process's executable folder.
To get the installation folder where wsladiag.exe is, use : wsl::windows::common::wslutil::GetMsiPackagePath()
15256c5 to
5e41ecf
Compare
src/windows/wsladiag/wsladiag.cpp
Outdated
| catch (...) | ||
| { | ||
| const auto hr = wil::ResultFromCaughtException(); | ||
| if (hr == E_INVALIDARG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend against this, because the service API might return that, and in that case we wouldn't want to show the usage.
To show command line parsing errors, I would recommend using something similar to this:
WSL/src/windows/common/WslClient.cpp
Line 1905 in cec99af
| auto strings = wsl::windows::common::wslutil::ErrorToString(context->ReportedError().value()); |
For the user errors to work, we'd need to explicitely enable them, like this:
wsl::windows::common::EnableContextualizedErrors(false);
And then create an execution context that we initialize in main (probably with new entrypoint value for wsladiag):
wsl::windows::common::ExecutionContext context{Context::WslaDiag};
Then we can use context to actually get the error string when an exception is thrown
test/windows/WsladiagTests.cpp
Outdated
| // Initialize the tests | ||
| TEST_CLASS_SETUP(TestClassSetup) | ||
| { | ||
| VERIFY_ARE_EQUAL(LxsstuInitialize(FALSE), TRUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specific to WSL and isn't needed for WSLA tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for LxsstuUninitialize(FALSE); below
| // Validate that list command output shows either no sessions message or session table | ||
| static void ValidateListOutput(const std::wstring& out) | ||
| { | ||
| const bool noSessions = out.find(L"No WSLA sessions found.") != std::wstring::npos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I was unclear in my previous comment. What I was suggesting was to have a method that takes the expected output as a parameter and do the validation. Something like:
void ValidateWslaDiagOutput(const std::string& Cmd, const std::string& ExpectedOutput);
So we can something like:
ValidateWslaDiagOutput("list", "<expected output>");
test/windows/WsladiagTests.cpp
Outdated
| TEST_METHOD(UnknownCommand_ShowsUsage) | ||
| { | ||
| auto [out, err, code] = RunWsladiag(L"blah"); | ||
| VERIFY_ARE_NOT_EQUAL(0, code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend validating that the error code is 1 explicitly, since that's what we expect
f08388e to
87b8180
Compare
src/windows/wsladiag/wsladiag.cpp
Outdated
| { | ||
| wsl::windows::common::EnableContextualizedErrors(false); | ||
|
|
||
| std::optional<ExecutionContext> context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: context is always set to the same value, so it doesn't need to be an std::optional
test/windows/WsladiagTests.cpp
Outdated
| auto [out, err, code] = RunWsladiag(L"blah"); | ||
| VERIFY_ARE_EQUAL(1, code); | ||
|
|
||
| const std::wstring combined = out + err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd recommend validating both fds separately. For instance error messages should always be on stderr, not stdout
test/windows/WsladiagTests.cpp
Outdated
| static void ValidateWslaDiagOutput(const std::wstring& cmd, const std::wstring& expectedSubstring) | ||
| { | ||
| auto [out, err, code] = RunWsladiag(cmd); | ||
| const std::wstring combined = out + err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I'd recommend having two parameters, one for each output so we can validate them separately.
I'll also recommend validating the entire strings, that way we'll catch if anything unexpected is printed
87b8180 to
c5a97ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive automated test coverage for the wsladiag CLI tool, including tests for argument parsing, command execution, error handling, and output validation. The implementation also includes refactored argument parsing to support hierarchical command structures, improved error reporting through ExecutionContext, and several code quality improvements.
Key changes:
- Added WsladiagTests.cpp with 8 test cases covering help, list, shell commands, and error scenarios
- Refactored argument parsing in wsladiag.cpp to use hierarchical parsing with the new stopUnknownArgs feature
- Integrated ExecutionContext for better error reporting and user-friendly error messages
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WsladiagTests.cpp | New test file with comprehensive smoke tests for wsladiag CLI tool, validating help output, list command, shell command error paths, and unknown command handling |
| test/windows/CMakeLists.txt | Added WsladiagTests.cpp to the test suite build |
| test/windows/WSLATests.cpp | Fixed typo in error message ("Comman" → "Command") |
| src/windows/wsladiag/wsladiag.cpp | Refactored argument parsing to use hierarchical parsing, added ExecutionContext for improved error reporting, added verbose logging for TTY configuration, improved console cleanup with logging, and explicitly initialized WSLA_PROCESS_FD.Path field |
| src/shared/inc/CommandLine.h | Added stopUnknownArgs parameter to ArgumentParser to support hierarchical command parsing; fixed typo in comment ("arvg" → "argv") |
| src/windows/common/ExecutionContext.h | Added WslaDiag context enum value for wsladiag execution context |
| src/windows/common/wslutil.cpp | Registered WslaDiag context string for error reporting |
| namespace WsladiagTests { | ||
| class WsladiagTests | ||
| { | ||
| WSL_TEST_CLASS(WsladiagTests) |
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test class uses WSL_TEST_CLASS macro which doesn't include wsladiag.exe in the BinaryUnderTest property list. While wslaservice.exe is included (which wsladiag depends on), the test is directly executing wsladiag.exe and should declare it as a binary under test. Consider adding a TEST_CLASS_PROPERTY for wsladiag.exe after the WSL_TEST_CLASS macro, similar to how other test classes declare their specific binaries.
| WSL_TEST_CLASS(WsladiagTests) | |
| WSL_TEST_CLASS(WsladiagTests) | |
| TEST_CLASS_PROPERTY(L"BinaryUnderTest", L"wsladiag.exe"); |
| std::wstring sessionName; | ||
| bool verbose = false; | ||
|
|
||
| ArgumentParser parser(std::wstring{commandLine}, L"wsladiag", 2); // Skip "wsladiag.exe shell" to parse shell-specific args |
Copilot
AI
Jan 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "Skip 'wsladiag.exe shell'" but the startIndex of 2 actually means the parser will skip the first 2 arguments (indices 0 and 1), which are "wsladiag.exe" (index 0) and "shell" (index 1). The comment is correct in intent but could be clearer. Consider rephrasing to "Skip first 2 args (wsladiag.exe, shell) to parse shell-specific args" to be more explicit about what startIndex=2 means.
| ArgumentParser parser(std::wstring{commandLine}, L"wsladiag", 2); // Skip "wsladiag.exe shell" to parse shell-specific args | |
| ArgumentParser parser(std::wstring{commandLine}, L"wsladiag", 2); // Skip first 2 args (wsladiag.exe, shell) to parse shell-specific args |
| { | ||
| printUsage(); | ||
| PrintUsage(); | ||
| return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using early return (no else block if there's a return in 'if' block) pattern here might be better for readability and reduced indentation
|
|
||
| if (sessionName.empty()) | ||
| { | ||
| THROW_HR(E_INVALIDARG); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do a wslutil::PrintMessage here and return 1
| L" wsladiag list\n" | ||
| L" wsladiag shell <SessionName> [--verbose]\n" | ||
| L" wsladiag --help\n", | ||
| stderr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage string could be made into a const std::string or a constexpr std::string_view. Then you can use that in the ValidateUsage() method in the WsladiagTests to verify if Usage string was returned as expected by the help case or an error case.
Summary of the Pull Request:
This PR adds automated test coverage for the wsladiag CLI tool under test/windows.
The new tests exercise the supported commands and common error paths to ensure argument parsing, exit codes, and output behavior remain correct.
PR Checklist
Detailed Description of the Pull Request / Additional comments
A new test file (WsladiagTests.cpp) is added to the Windows test suite. The tests invoke wsladiag.exe directly and validate:
The tests intentionally avoid interactive shell success cases and distro-dependent scenarios to keep the suite stable and fast. Assertions use substring matching rather than exact formatting to reduce flakiness.
Validation Steps Performed
Built the solution and confirmed wsladiag.exe is generated correctly